repo: allow_missing: Compare hash_info.value instead of hash_info #9839
Conversation
| @@ -314,7 +314,8 @@ def _changed_deps( | |||
| if status: | |||
| if allow_missing and status[str(dep)] == "deleted": | |||
| if upstream and any( | |||
| dep.fs_path == out.fs_path and dep.hash_info != out.hash_info | |||
| dep.fs_path == out.fs_path | |||
| and dep.hash_info.value != out.hash_info.value | |||
There was a problem hiding this comment.
@pmrowla do you think it is ok?
See the added test / linked issue for the scenario, is there something different we should do for 2.X .dvc files that are tracked as deps in 3.X dvc.lock files?
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| dep.fs_path == out.fs_path | ||
| and dep.hash_info.value != out.hash_info.value |
There was a problem hiding this comment.
We should not look into hash_info.value. 2.0 and 3.0 hashes are not compatible.
There was a problem hiding this comment.
Yes, I know the theory:
Do we care in practice (today, not if we introduce different hashing algorithm)?
I mean, isn't the same chance of a collision as today with 2 different 3.0 md5 hashes?
There was a problem hiding this comment.
Even without considering the collision, if I am understanding the branch of this code correctly, there is a chance of hashes being mismatched and returning modified, no?
There was a problem hiding this comment.
@daavoo hash_info should be compared as a whole, comparing only values without name looks like a hack. Could you elaborate on why you need to do it this way?
There was a problem hiding this comment.
I guess new output is getting hashed as 3.x and deps is hashed as 2.x format?
There was a problem hiding this comment.
Seems like it would be better to just ask to upgrade (e.g. with dvc commit) #9818 (comment) Otherwise we have to compute both hashes (which is undesirable) or hack like it is done in this PR, which makes it straight up error-prone.
There was a problem hiding this comment.
@daavoo hash_info should be compared as a whole, comparing only values without name looks like a hack.
It doesn't look, it is a hack 😄
Could you elaborate on why you need to do it this way?
It is in the added test / linked issue.
If there is 2.X .dvc referenced as dep in a 3.X dvc.lock, the hash_info comparison fails because of has_info.name
There was a problem hiding this comment.
there is a chance of hashes being mismatched and returning modified, no?
Not sure I follow, sorry.
This is what happens before the P.R: an equivalent folder is mismatched because of hash_info.name but only when the content is missing. If you pull the data, the same .dvc and dvc.lock report it as not modified
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #9839 +/- ##
==========================================
+ Coverage 90.81% 90.83% +0.01%
==========================================
Files 473 473
Lines 35982 35988 +6
Branches 5189 5189
==========================================
+ Hits 32678 32688 +10
+ Misses 2713 2710 -3
+ Partials 591 590 -1
☔ View full report in Codecov by Sentry. |
skshetry
changed the title
skshetry
changed the title
|
Discussed to follow a different approach:
|
skshetry
deleted the
9818-dvc-repro-dry-allow-missing-fails-on-missing-data
branch
Closes #9818